Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: filterReq crashes send middleware #3

Merged
merged 4 commits into from
Jul 13, 2021
Merged

fix: filterReq crashes send middleware #3

merged 4 commits into from
Jul 13, 2021

Conversation

langtutheky
Copy link
Contributor

Issue

fix: filterReq crashes send middleware

Details

When filterReq return false, the middleware will crash with this error after it attempts to call next to static serving middleware such as send

error: Uncaught Error: The response is not writable.
throw new Error("The response is not writable.");

Must return the promise yield by next when invoking from within a promise chain. This is similar to Getting "The response is not writable."

CheckList

  • PR starts with [#ISSUE_ID].
  • Has been tested (where required) before merge to main.

@cmorten
Copy link
Owner

cmorten commented Jul 9, 2021

Hi @langtutheky 👋 thanks for the fix!

I was wondering if you would be able to add a test for this to https://github.com/asos-craigmorten/oak-http-proxy/blob/main/test/filterReq.test.ts ?

There are already cases for when false is returned so be interesting to see what is required to make this error!

@langtutheky
Copy link
Contributor Author

@cmorten You are welcome, this is actually a major design problem/bug from Oak. I will let them know later but just keep in mind that for now, all call to next must directly participate in the middleware chain through either return or await.

@cmorten
Copy link
Owner

cmorten commented Jul 10, 2021

@langtutheky tests look good - just need to run make fmt to fix some formatting issues

asos-craigmorten
asos-craigmorten reviewed Jul 13, 2021
View reviewed changes
Copy link
Collaborator

@asos-craigmorten asos-craigmorten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the issue now I believe is conflicting std lib versions, potentially because of the upgrade of Oak. We can either downgrade Oak back to 7.3.0 to try and get this out, or update the various versions ( in this or another PR - leaning towards getting something separate raised and merged ).

@langtutheky
Copy link
Contributor Author

I think you should make another PR to upgrade Oak to 7.7.0, there was a type error while running test with 7.3.0 which is why I upgraded. I can only run 7.3.0 with --no-check flag, which is not ideal.

@cmorten cmorten merged commit c9d85e1 into cmorten:main Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asos-craigmorten asos-craigmorten asos-craigmorten left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@langtutheky @cmorten @asos-craigmorten